[SDSP-485] Support suppressions for secret rules - #938
gh-worker-dd-mergequeue-cf854d[bot] merged 4 commits into
Conversation
| exact_match.sort(); | ||
|
|
||
| digest.push_str(&format!( | ||
| ":suppressions:{starts_with:?}:{ends_with:?}:{exact_match:?}", |
There was a problem hiding this comment.
I wouldn't rely on the formatting of the Debug impl (which is what :? is) to format the contents.
I would use starts_with.join and then use the normal Display impl ({starts_with})
| })) | ||
| .generate_diff_aware_digest(); | ||
|
|
||
| assert_ne!(legacy_digest, with_suppressions); |
There was a problem hiding this comment.
Not sure what value this adds. I would remove this test
| } | ||
|
|
||
| #[test] | ||
| fn test_convert_to_sds_ruleconfig_with_suppressions() { |
There was a problem hiding this comment.
This test isn't testing anything. You should either have an assertion somewhere or remove the test.
There was a problem hiding this comment.
I was more or less using the same pattern as the test above to ensure that converting the rule doesn't panic but I agree this is probably not testing much of anything. I'm removing this
| } | ||
|
|
||
| #[test] | ||
| fn test_find_secrets_with_rule_suppressions() { |
There was a problem hiding this comment.
Not sure I understand the point of this test -- could you explain?
There was a problem hiding this comment.
The test is checking that the built scanner from a SecretRule that has suppressions configured does in fact suppress matches.
Here the pattern would match both lines in the dummy code provided, but the second line has an exact_match in the suppressions that would suppress it. The test makes sure that this is the case and that we indeed match the first line, but not the second.
There was a problem hiding this comment.
My concern is that the behavior it asserts:
assert_eq!(
matches.first().unwrap().matches.first().unwrap().start,
Position { line: 1, col: 1 }
);Is merely testing that SDS's suppression implementation is correct. That belongs in SDS library tests, not here (and in fact, this is already covered by SDS's test_match_suppression_suppress_half_of_the_matches)
And additionally, we have no test coverage for what new behavior this PR actually introduces -- that build_sds_scanner correctly threads the SecretRule suppressions through to SDS. Hypothetically if we (datadog-static-analyzer) forgot to pass the suppression to SDS, but SDS were to independently omit the second finding for whatever reason, this test would incorrectly pass.
So what we need is a control case. You'd want to test a SecretRule { suppressions: None } and assert assert_eq!(matches[0].matches.len(), 2); and then pass in SecretRule { suppressions: Some(...) } and assert_eq!(matches[0].matches.len(), 1);
And from there we know the delta can only be explained by the change to suppressions.
There was a problem hiding this comment.
Ah yes that's very fair, thank you for the thorough explanation. I've reworked the test in 0af2092 to match the behavior you are proposing.
| pub exact_match: Vec<String>, | ||
| } | ||
|
|
||
| impl From<&SecretRuleSuppressions> for dd_sds::Suppressions { |
There was a problem hiding this comment.
It's more idiomatic to impl From<SecretRuleSuppressions> and let the caller control cloning if they want to.
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
No unresolved blocking issues were identified.
Review effort: Lite
Findings: None
What changed in this PR
Adds secret-rule suppressions to exclude configured dummy or test values from secret scanning.
Changes:
- Adds suppression configuration and SDS conversion.
- Propagates suppressions through API models and scanning.
- Updates tests and fixtures.
| File | Description |
|---|---|
crates/secrets/src/scanner.rs |
Adds suppression behavior coverage. |
crates/secrets/src/model/secret_rule.rs |
Defines and applies suppressions. |
crates/cli/src/sarif/sarif_utils.rs |
Updates test rule fixtures. |
crates/cli/src/model/datadog_api.rs |
Deserializes API suppressions. |
crates/bins/src/git_history.rs |
Updates test fixtures. |
crates/bins/src/bin/datadog_static_analyzer_server/endpoints.rs |
Updates server test fixtures. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
gh-worker-dd-devflow-36fce6
Bot
added
mergequeue-status: queued
mergequeue-status: in_progress
and removed
mergequeue-status: queued
labels
What problem are you trying to solve?
Some secrets matched by secret rules aren't actual secrets: they may be test tokens, test card numbers, dummy values, etc.
What is your solution?
Add support for suppressions, which are natively supported by the secret scanning engine already. This allows users to specify "suppressions" which are some kind of filter on top of the matches, to determine whether the match should be kept or not.